-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add testing on Windows, macOS, and R 4.2 #545
base: main
Are you sure you want to change the base?
Conversation
There seems to be some kind of issue occasionally. I sometimes see this locally
and one of our builds above shows it too |
hmm that's surprising these dummy frontend sockets are on ark/crates/amalthea/src/fixtures/dummy_frontend.rs Lines 88 to 95 in 77d9400
Since you can see it locally, maybe you could set a breakpoint and explore with e.g. |
310b15a
to
5a6db98
Compare
And also do this for the harp test binary, which starts R and tests UTF-8 related capabilities.
5a6db98
to
d0a4f7b
Compare
bin_edges <- as.POSIXct(h$breaks, tz = attr(x, "tzone")) | ||
# Must supply an `origin` on R <= 4.2 | ||
origin <- as.POSIXct("1970-01-01", tz = "UTC") | ||
bin_edges <- as.POSIXct(h$breaks, tz = attr(x, "tzone"), origin = origin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug found when adding 4.2 CI
# Must open in binary write mode, otherwise even though we set | ||
# `eol = "\n"` on Windows it will still write `\r\n` due to the C | ||
# level `vfprintf()` call. | ||
con <- file(path, open = "wb") | ||
defer(close(con)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug found when adding Windows CI. Tests expect \n
as the line ending
And the way we do readChar(path, file.info(path)$size - 1L, useBytes = TRUE)
above expects 1 char for the trailing line (i.e. \n
not \r\n
)
// 10 second timeout on `recv()` operations (for testing for now) | ||
// TODO: Expose `IS_TESTING` in its own lightweight crate? | ||
if let Err(error) = socket.set_rcvtimeo(10000) { | ||
return Err(Error::CreateSocketFailed(name, error)); | ||
} | ||
if let Err(error) = socket.set_sndtimeo(10000) { | ||
return Err(Error::CreateSocketFailed(name, error)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Remove or hide behind test flag
The goal is to avoid an infinite hang on send/recv issues during kernel tests
fn test_locale() { | ||
// These tests assert that we've embedded our Application Manifest file correctly in `build.rs` | ||
r_test(|| { | ||
let latin1 = harp::parse_eval_base("l10n_info()$`Latin-1`").unwrap(); | ||
let latin1 = bool::try_from(latin1).unwrap(); | ||
assert!(!latin1); | ||
|
||
let utf8 = harp::parse_eval_base("l10n_info()$`UTF-8`").unwrap(); | ||
let utf8 = bool::try_from(utf8).unwrap(); | ||
assert!(utf8); | ||
|
||
let codepage = harp::parse_eval_base("l10n_info()$codepage").unwrap(); | ||
let codepage = i32::try_from(codepage).unwrap(); | ||
assert_eq!(codepage, 65001); | ||
|
||
let system_codepage = harp::parse_eval_base("l10n_info()$system.codepage").unwrap(); | ||
let system_codepage = i32::try_from(system_codepage).unwrap(); | ||
assert_eq!(system_codepage, 65001); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests on both the ark and harp side that the application manifest was correctly embedded into the test binary
#' Write out templated Windows Application Manifest files | ||
#' | ||
#' We use two files to embed an application manifest on Windows: | ||
#' - `{crate}-manifest.rc` | ||
#' - `{crate}.exe.manifest` | ||
#' | ||
#' And we generate these two files for both `harp` and `ark`. | ||
#' | ||
#' They are effectively the same, we just need to swap out the crate names, | ||
#' so we use this script to write the files in a consistent manner. | ||
write_manifest <- function(root, crate) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the manifest files are almost identical between ark and harp, we generate them from templates
This PR has the goal of getting us the following CI structure:
Adding support for macOS was incredibly simple.
Adding support for R 4.2 revealed one bug that I've fixed and called out below.
Adding support for Windows has been a nightmare. Further details below.
You should be able to trigger each OS's workflow manually through workflow dispatch, and you should also be able to turn on SSH-ing into the Linux workflow (it will pause right before running
cargo test
).Application manifest
I noticed that during tests on Windows,
sessionInfo()
reports that we are not in a UTF-8 locale and we are on Windows Server 2012, even though the github machine is Windows Server 2022. This is a classic sign that our application manifest file wasn't being used #178.The manifest is being used for our main binary,
ark.exe
, but it wasn't being embedded into our test binary, likeark-<stuff>.exe
. Making that happen was a little complicated. The crux of it is:embed_resource::compile()
setscargo:rustc-link-arg-bins
, which doesn't target test binariesembed_resource::compile_for_tests()
setscargo:rustc-link-arg-tests
, but that doesn't target unit test binaries due to a Rust bugembed_resource::compile_for_everything()
setscargo:rustc-link-ark
, which is the "just do it for everything" approach, and that does work. This didn't exist though, I had to ask the embed-resource maintainer for it!So that allowed ark's unit tests to start up R in a way that R was in a UTF-8 locale and ran under Windows Server 2022.
But harp also starts R for its unit tests! And it also tests UTF-8 behavior! So I had to add a
build.rs
script to harp to also embed the manifest file there as well.Open comms timing issues
See #548, I seemed to hit this error every now and then on the Windows CI and my local Windows VM
Outstanding issues
Occasionally the test suite will hang on windows after running
test_kernel
, but I cannot figure out why. I get no information except eventually it says something like:My guess is that a zmq
send()
orrecv()
is stuck, so I've setset_sndtimeo()
andset_rectimeo()
to try and turn these cases into a panic at test time, but I actually haven't been able to reproduce the issue since I added them, and can't reproduce this one locally.